Improved aggrMethod to accept multiple values #605

Madhu1029 · 2023-02-22T12:28:49Z

Fixes #432 partially.
aggrMethod can take multiple aggregations using comma separation.

Madhu1029 · 2023-03-16T06:49:49Z

If PR seems ok, please merge the PR.
Thanks

fgalan · 2023-04-04T05:50:06Z

CHANGES_NEXT_RELEASE

@@ -1 +1,2 @@
+- Improved aggrMethod to accept multiple values (#432, partial)


Suggested change

- Improved aggrMethod to accept multiple values (#432, partial)

- Add: aggrMethod accept multiple values separated by comma (#432, partial)

- Add: aggrMethod 'all' to get all the possible aggregations (#432, partial)

In addition, documentation at doc/manuals/aggregated-data-retrieval.md (aggrMethod bullet) should be modified to explain the new functionality.

Resolved in ee9cded

fgalan · 2023-04-04T13:44:16Z

test/unit/sthTestUtils.js

@@ -912,21 +912,22 @@ function aggregatedDataAvailableSinceDateTest(ngsiVersion, params, done) {
            break;
    }

-    let value;


Instead of modifying existing tests, please add new ones. In particular, I'd suggest two new tests cases:

Using the 'all' token

Using two aggregation methods (whatever) separated by ","

Hi @fgalan,

I have added the new test cases here.

Previously, only one aggrMethod was there so switch case was working fine. But in case of multiple aggregation method we need to update the code for multiple values separately. The above code change is for util class to accommodate the multiple aggregation methods.

Madhu1029 · 2023-05-12T11:06:34Z

Hi @fgalan ,
I have updated the docs. Please merge this PR if it seems ok.

Thanks

fgalan · 2023-06-01T14:24:47Z

lib/server/sthServer.js

+        const list = ['min', 'max', 'sum', 'sum2', 'occur', 'all'];
+        const joinedList = '(' + list.join('|') + ')';
+        const regex = new RegExp('^' + joinedList + '(,' + joinedList + ')*$');


Please use more meaningful names names for the variables. For instance:

Suggested change

const list = ['min', 'max', 'sum', 'sum2', 'occur', 'all'];

const joinedList = '(' + list.join('|') + ')';

const regex = new RegExp('^' + joinedList + '(,' + joinedList + ')*$');

const aggList = ['min', 'max', 'sum', 'sum2', 'occur', 'all'];

const joinedAggList = '(' + aggList.join('|') + ')';

const aggRegex = new RegExp('^' + joinedAggList + '(,' + joinedAggList + ')*$');

Fixed in 3b51b4c

fgalan · 2023-06-01T14:25:15Z

doc/manuals/aggregated-data-retrieval.md

-    provided by these aggregated methods with the number of samples, it is possible to calculate probabilistic values
-    such as the average value, the variance as well as the standard deviation. It is a mandatory parameter.
+    samples) for numeric attribute values and `occur` for attributes values of type string. It accepts multiple values
+    separated by comma (min,max) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used


Suggested change

separated by comma (min,max) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used

separated by comma (eg. `aggrMethod=min,max`) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used

Fixed in 3b51b4c

fgalan · 2023-06-01T14:39:09Z

test/unit/sth_test.js

@@ -324,13 +324,45 @@ describe('sth tests', function() {
            })
        );

+        it(


I'd suggest to add a couple of cases:

should respond with 400 - Bad Request if aggMethod is not a valid one (e.g. aggMethod=foo)

should respond with 400 - Bad Request if aggMethod mixes a valid one with a not valid one (eg. aggMethod=max,foo)

Btw, what happens if aggMethod mixes all with another method? Eg. aggMethod=all,min? Do we get and error or the "min" part is ignored and that's equivalent to aggMethod=all?

Added test cases in 3b51b4c

If aggMethod mixes all with another method e.g. aggMethod=all,min, then it is equivalent to aggMethod=all ignoring all other parameters.

Madhu1029 · 2023-10-03T04:54:36Z

Hi @fgalan ,

I have updated all the suggestions mentioned by you in previous comment.
I request you to please review the PR.

fgalan · 2023-10-03T15:08:47Z

Hi @fgalan ,

I have updated all the suggestions mentioned by you in previous comment. I request you to please review the PR.

Thanks for your contribution! We would have a look to the PR the soon as possible.

Madhu1029 · 2024-04-12T04:21:16Z

Hi @fgalan ,

Could you please have a look on this PR?

AlvaroVega · 2024-04-12T09:50:25Z

Is this a new feature or extends and modify a current one?
Should documentation be updated ?

Madhu1029 · 2024-04-12T09:56:10Z

Is this a new feature or extends and modify a current one? Should documentation be updated ?

Hi @AlvaroVega ,
Thanks for your response.
Currently, we can only specify single aggrMethod like aggrMethod=sum or aggrMethod=min but with this PR we can specify multiple aggrMethod in single go like aggrMethod=sum,min,max. In addition to this, aggrMethod=all will give all the aggregation method results in one go.

Documentation for this is already updated in this PR.

fgalan

LGTM

AlvaroVega

LGTM

fgalan · 2024-11-22T12:28:30Z

@Madhu1029 thanks for your contribution! Sorry for the delay merging this PR :)

Improved aggrMethod to accept multiple values

e53b905

fgalan reviewed Apr 4, 2023

View reviewed changes

Updated docs for aggrMethod

ee9cded

fgalan reviewed Jun 1, 2023

View reviewed changes

Merge branch 'telefonicaid:master' into issue432_aggrMethod_partial

c3601a5

fgalan mentioned this pull request Sep 14, 2023

How to configure CORS in docker environment variables #608

Open

Madhu1029 and others added 3 commits September 22, 2023 11:24

Merge branch 'telefonicaid:master' into issue432_aggrMethod_partial

e2fcabd

Updated 400 Bad Request test cases

3b51b4c

Fixed lint error

885c78c

Merge branch 'master' into issue432_aggrMethod_partial

4abf35f

fgalan approved these changes Nov 22, 2024

View reviewed changes

AlvaroVega approved these changes Nov 22, 2024

View reviewed changes

AlvaroVega merged commit 359b095 into telefonicaid:master Nov 22, 2024
6 checks passed

This was referenced Nov 22, 2024

Improve aggrMethod #432

Open

FIX more accurate CHANGES_NEXT_RELEASE #640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved aggrMethod to accept multiple values #605

Improved aggrMethod to accept multiple values #605

Madhu1029 commented Feb 22, 2023

Madhu1029 commented Mar 16, 2023

fgalan Apr 4, 2023

Madhu1029 May 2, 2023

fgalan Apr 4, 2023

Madhu1029 May 2, 2023

Madhu1029 commented May 12, 2023

fgalan Jun 1, 2023

Madhu1029 Sep 22, 2023

fgalan Jun 1, 2023

Madhu1029 Sep 22, 2023

fgalan Jun 1, 2023

Madhu1029 Sep 22, 2023

Madhu1029 commented Oct 3, 2023

fgalan commented Oct 3, 2023

Madhu1029 commented Apr 12, 2024

AlvaroVega commented Apr 12, 2024

Madhu1029 commented Apr 12, 2024

fgalan left a comment

AlvaroVega left a comment

fgalan commented Nov 22, 2024

		@@ -1 +1,2 @@
		- Improved aggrMethod to accept multiple values (#432, partial)

	- Improved aggrMethod to accept multiple values (#432, partial)
	- Add: aggrMethod accept multiple values separated by comma (#432, partial)
	- Add: aggrMethod 'all' to get all the possible aggregations (#432, partial)

	separated by comma (min,max) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used
	separated by comma (eg. `aggrMethod=min,max`) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used

@@ @@ -324,13 +324,45 @@ describe('sth tests', function() { @@
                           })
                       );
+                      it(

Improved aggrMethod to accept multiple values #605

Improved aggrMethod to accept multiple values #605

Conversation

Madhu1029 commented Feb 22, 2023

Madhu1029 commented Mar 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu1029 commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu1029 commented Oct 3, 2023

fgalan commented Oct 3, 2023

Madhu1029 commented Apr 12, 2024

AlvaroVega commented Apr 12, 2024

Madhu1029 commented Apr 12, 2024

fgalan left a comment

Choose a reason for hiding this comment

AlvaroVega left a comment

Choose a reason for hiding this comment

fgalan commented Nov 22, 2024